Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix tests after changes in maude #122

Merged
merged 2 commits into from
Mar 30, 2022
Merged

Fix tests after changes in maude #122

merged 2 commits into from
Mar 30, 2022

Conversation

javierggt
Copy link
Contributor

@javierggt javierggt commented Mar 24, 2022

Description

It seems like in the latest maude update, some floating point values changed slightly. This caused chandra_aca tests to fail. This PR changes those tests so floating point values are checked using np.isclose(a, b) instead of a == b.

Testing

  • Passes unit tests linux (at least one required)
  • [n/a] Functional testing

Fixes #

@jeanconn
Copy link
Contributor

Are there any downstream issues with these differences from the maude update?

@javierggt
Copy link
Contributor Author

I don't know for sure, but I doubt it very much. The issue was that the values are not exactly the same, which is not uncommon with floating point types. The question is whether these tests need to be to full precision.

@jeanconn
Copy link
Contributor

Seems reasonable to me too, though I've forgotten what the default isclose tolerances are.

@jeanconn jeanconn requested a review from taldcroft March 27, 2022 14:10
@@ -63,7 +63,8 @@ def test_vcdu_0_raw():

ref_data = test_data['686111007-686111017']['raw']
data = maude_decom.get_raw_aca_packets(686111007, 686111017)
for key in ['TIME', 'MJF', 'MNF']:
assert np.all(np.isclose(data['TIME'], ref_data['TIME']))
Copy link
Member

@taldcroft taldcroft Mar 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jeanconn's point was a good one -- np.isclose uses a default relative tolerance of (IIRC) 1e-6, so that this ends up being a large number here. Best to set rtol=0, atol=1e-3 so we test the times are matching to a msec.

@@ -203,7 +204,10 @@ def test_vcdu_vs_level0():
table2 = maude_decom._get_aca_packets(raw, start, stop,
combine=True, adjust_time=True, calibrate=True)
for col, col2 in zip(table.itercols(), table2.itercols()):
assert np.all(col == col2)
if col.name in ['TIME', 'END_INTEG_TIME']:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated this to just do the 1e-3 atol on the time columns.

@jeanconn jeanconn requested a review from taldcroft March 28, 2022 21:13
@javierggt
Copy link
Contributor Author

I forgot about this PR. I just checked that tests pass after @jeanconn and will merge+tag now.

Copy link
Member

@taldcroft taldcroft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@javierggt javierggt merged commit 8e1e974 into master Mar 30, 2022
@javierggt javierggt mentioned this pull request Mar 30, 2022
@javierggt javierggt mentioned this pull request Aug 3, 2022
@javierggt javierggt deleted the fix-tests branch February 27, 2023 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants